-
Notifications
You must be signed in to change notification settings - Fork 227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace obsolete react context #663
Replace obsolete react context #663
Conversation
@redonkulus I gave it a try today. I just realized that there is another breaking change: since we are not using the old context anymore, we can't have in our API the feature of injecting other context values besides fluxible ones: connectToStores // before
function connectToStores(Component, stores, getStateFromStores, customContextTypes)
// after
function connectToStores(Component, stores, getStateFromStores) provideContext // before
function provideContext(Component, customContextTypes)
// after
function provideContext(Component) Would it be a problem? By the way, besides injecting react context values, // before
function provideContext(Component, customContextTypes)
// after
function provideContext(Component, pluginsKeys)
provideContext(Component, ['plugin1', 'plugin2']) What do you think? (was I clear enough?) |
I think that makes sense as long as users can still add to the context. Otherwise, we would have to recommend an alternative method. @lingyan can you remember often we add custom context values, I know plugins do, seems like this would be a common thing. I would recommend testing this in the doc site to see if any other issues come up. Unrelated, but it would be good to migrate to import / export in these files too. This would help with tree shaking as well. |
@redonkulus Another breaking change while updating fluxible-router: NavLink use to have a possibility to use a custom logger through contextTypes as well. I removed that so I could consume fluxible-context easily inside the class. Would that be fine? |
I didn't get what makes sense. I see 3 possibilities here:
For the commonjs issue, I've just create another PR (#664 ) |
8d2ddf9
to
b3a3c36
Compare
@redonkulus I've added back the |
@pablopalacios Sounds good. Can you start a MIGRATATION.md doc in this branch to keep track of the breaking changes and what the code changes will be? |
7170449
to
1070ad9
Compare
@redonkulus I've removed By the way, would it be possible to rebase the 1.x-dev branch on top of master? |
db1f963
to
145798d
Compare
@pablopalacios Ok I've rebased and pushed, please update your fork. |
1070ad9
to
0a51a12
Compare
In this way we can still inject custom data from plugins into the state.
0a51a12
to
2621d7d
Compare
@redonkulus, @lingyan any comments here? |
Sorry, I will get this merged and released today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me, thanks for all the work you are doing!
Closes #584
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.